Make compiler hint to import unqualified types/values #4304
Conversation
|
The hints should be done now, although I don't if its possible to find the arity (suppose someone wrote |
There was a problem hiding this comment.
Thank you! I've added some comments inline, and snapshot tests will need to be added to test all the new paths through the code.
It would be great to also check the arity of the constructors. Suggesting a zero-arity Wibble when it is being used in a way that takes 2 arguments would be unhelpful.
| // Don't suggest importing modules if they are already imported | ||
| _ if self | ||
| .imported_modules | ||
| .contains_key(importable.split('/').last().unwrap_or(importable)) => |
There was a problem hiding this comment.
There's no fixed relationship between the name used once imported and the name of a module, and this code will produce the same name for multiple modules, so this doesn't successfully skip over already imported modules.
There was a problem hiding this comment.
To be fair I had just copied that line from the suggest_modules function in the environment module, think it'd be correct. I think I should remove the later code where it adds the imported modules to the suggestions vector, although I don't know how I'd detect if a module is imported or not
compiler-core/src/type_/error.rs
Outdated
| if is_type { | ||
| format!("Did you mean to import `{module}.{{type {name}}}`?") | ||
| } else { | ||
| format!("Did you mean to import `{module}.{{{name}}}`") |
There was a problem hiding this comment.
I don't want to encourage unqualified importing of values. Perhaps we should only make these suggestions for types and record constructors.
There was a problem hiding this comment.
Okay! Tying with my point below, we could write the "generic" value case as "Did you mean to reference module.{value}" and, in case it isn't already imported, "Did you mean to import module and reference module.{value}". What do you think?
| @@ -23,3 +23,4 @@ error: Unknown type | |||
| The type `Wibble` is not defined or imported in this module. | |||
| There is a value in scope with the name `Wibble`, but no type in scope with | |||
| that name. | |||
| Hint: Did you mean to import `module.{type Wibble}`? | |||
There was a problem hiding this comment.
I think the ` should be before the import keyword?
There was a problem hiding this comment.
I think if we make the "import" part of the "code" section (between backticks), it should probably be written "Did you mean to write code here". What do you think?
There was a problem hiding this comment.
Actually, the current code for module suggestions (in the qualified case) says "Did you mean to import module", so how it is right now should be more consistent
compiler-core/src/error.rs
Outdated
| @@ -2326,7 +2331,7 @@ but no type in scope with that name." | |||
| Diagnostic { | |||
| title: "Unknown variable".into(), | |||
| text, | |||
| hint: None, | |||
| hint: suggestions.first().map(|suggestion| suggestion.suggestion(name, false)), | |||
There was a problem hiding this comment.
There's a vector of suggestions, but only the first one is used. Seems like only one should be given in that case.
There was a problem hiding this comment.
I thought this was fine, as that is what the current code for module suggestions does. Anyway, I'll change it!
compiler-core/src/type_/error.rs
Outdated
| } | ||
|
|
||
| impl TypeOrVariableSuggestion { | ||
| pub fn suggestion(&self, name: &str, is_type: bool) -> String { |
There was a problem hiding this comment.
Never use bools as arguments please 🙏 This could be a Layer.
There was a problem hiding this comment.
To be frank, I did think that bool wasn't a good fit, although I didn't know what to name the Enum as. Thanks!
|
The code should now consider arity, but it is an |
This PR closes #4297
Currently the way it decides which module to hint is by preferring imported modules over importable ones, and sorting by name length.